fix(hnsw): reject index files whose vector count overflows size_t#332
Conversation
|
The regression test does not specifically exercise the new vector-size overflow guard. Only the header is patched: |
f1897d0 to
b8d2f5d
Compare
|
Thank you. The header-patch test couldn't reach the vectors guard (the oversized node-level read fails first), so it didn't distinguish the path. I factored the check into ray_hnsw_vec_size_valid(n_nodes, dim) and replaced the test with a direct unit test (ordinary dims, non-positive, an overflowing pair, and the exact SIZE_MAX/sizeof(float) boundary); it fails if the guard logic regresses. Amended in b8d2f5d. |
hnsw_load_impl read n_nodes and dim straight from the file header and sized the vectors allocation as n_nodes * dim * sizeof(float) with no overflow check. A crafted header could make that product wrap size_t, so ray_sys_alloc under-allocated the buffer while the following fread still read the full (large) element count and wrote past the allocation — a heap-overflow write driven by an untrusted index file. Factor the check into ray_hnsw_vec_size_valid(n_nodes, dim) and reject the header before any allocation, mirroring the per-layer neighbor guard. Add a unit test that drives the helper directly (ordinary dims, non-positive dims, an overflowing pair, and the exact size_t boundary). It is tested at the helper rather than through ray_hnsw_load because an overflow-patched header is refused earlier — the huge node-level read fails first — so a full-load test could not distinguish the guard.
b8d2f5d to
5735507
Compare
3 participants
What & why
ray_hnsw_load/ray_hnsw_mmapreadn_nodesanddimdirectly from theon-disk index header and sized the vectors allocation as
n_nodes * dim * sizeof(float)with no overflow check (the vectors block ofhnsw_load_implinsrc/store/hnsw.c). A crafted header can make that productwrap
size_t, soray_sys_allocunder-allocates the buffer while the followingfread(vecs, sizeof(float), vec_count, f)still reads the full (large) elementcount — writing past the allocation. That is a heap-overflow write driven
entirely by an untrusted index file (an attacker who can supply a matching
hnsw_header.bin+hnsw_vectors.bin).The per-layer neighbor arrays a few lines down already carry a divide-based
overflow guard; the vectors block was missing the equivalent. This adds the same
guard in header validation, so an overflowing file is rejected before any
allocation:
n_nodesanddimare already validated> 0at that point, so the divide issafe.
Test
embedding/hnsw_load_overflow_header_rejectedsaves a valid index, patches theheader's
n_nodes/dimto an overflowing pair, and asserts the load is refused.It lives at the C layer rather than under
test/rfl/because it needs byte-levelmanipulation of the binary header, which the behavioural harness can't express.
Full suite under the debug ASan + UBSan build:
3632 of 3633 passed (1 skipped, 0 failed).Checklist
dev(notmaster)fix:)makebuilds cleanly (no new warnings)make testpasses; tests added/updated for behaviour changes